-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve animation issues with Date picker #75
Conversation
disc0infern0
commented
Dec 8, 2021
•
edited by peterfriese
Loading
edited by peterfriese
- Refactored date/time pickers to new view and updated control of state with new enum, allowing animations of picker view transitions to be handled precisely with new function setPickerState.
- No settings for transitions or animations work well (in a Form) when transitioning from a state where the date picker is shown, so use the setPickerState function to make transitions from a date picker shown state without animation, but animate all other view transitions.
- There is now no janky animation whatsoever, but 2 out of the 6 possible transitions now happen without animation.
… ReminderDetailsView
… a new view; ReminderDateTimeView, and passed viewModel via the environment.
…nalAnimation to honour Accessibility setting
… with new enum, allowing animations of picker view transitions to be handled precisely with new function setPickerState. No settings for transitions or animations work well (in a Form) when transitioning from a state where the date picker is shown, so use the setPickerState function to make transitions from a date picker shown state without animation, but animate all other view transitions. There is now no janky animation whatsoever, but 2 out of the 6 possible transitions now happen without animation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments regarding formatting, code structure, etc. Will take a closer look at the functionality later today.
@@ -7,6 +7,8 @@ | |||
objects = { | |||
|
|||
/* Begin PBXBuildFile section */ | |||
49281E992760EBCB0046A465 /* ReminderDateTimeView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 49281E982760EBCB0046A465 /* ReminderDateTimeView.swift */; }; | |||
49281E9A2760EBCB0046A465 /* ReminderDateTimeView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 49281E982760EBCB0046A465 /* ReminderDateTimeView.swift */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate entries, might be a result of manually merging this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On closer inspection, the two lines are not duplicates: line 10 starts: 49281E99 vs line 11 starting: 49281E9A
The last character is different. Just to be sure, I ran 'uniq -d project.pbxproj' from the command line, and there was no output - indicating no duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
code/frontend/MakeItSo/Shared/Features/ReminderList/ViewModels/ReminderDetailsViewModel.swift
Outdated
Show resolved
Hide resolved
code/frontend/MakeItSo/Shared/Features/ReminderList/ViewModels/ReminderDetailsViewModel.swift
Outdated
Show resolved
Hide resolved
code/frontend/MakeItSo/Shared/Features/ReminderList/Views/ReminderDateTimeView.swift
Show resolved
Hide resolved
code/frontend/MakeItSo/Shared/Features/ReminderList/Views/ReminderDateTimeView.swift
Show resolved
Hide resolved
…pe if else formatting. Updated one instance of newvalue to newValue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had an opportunity to check out how this looks like on the Simulator:
CleanShot.2021-12-09.at.08.57.32.mp4
As becomes obvious from the screen recording, this is nowhere near the smooth transitions of the original app.
As long as we're not able to get close to the original, I am very reluctant to make wide-ranging changes to the code that will make it more difficult to read. Plus, there is hope that Apple might actually fix this in the SDK itself, and I'd like to prevent us having to roll back a lot of changes we only made to workaround an issue.
Feel free to keep this PR open and continue optimising it, and use it as a reference when filing feedback to the SwiftUI team.
Removed unused Date functions and repositioned to end of file with additional commentary. Moved definition of enum for PickerState into the class. RemindersListViewModel.swift: Removed unneeded delay in combine pipeline for removing blank entries.